Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow warnings during local development #271

Merged
merged 1 commit into from
Feb 9, 2021

Conversation

jrvanwhy
Copy link
Collaborator

@jrvanwhy jrvanwhy commented Feb 2, 2021

This keep's CI's behavior where it errors if libtock-rs has warnings.

This is a second attempt at PR #270. We can't specify RUSTFLAGS directly because doing so overrides all rustflags configurations in .cargo/config. Those rustflags configs are important because they configure relocation and set the linker script. Instead, I append the "deny warnings" config to .cargo/config in CI itself.

I tested this by opening two PRs in my fork: one to test that this PR still builds, and one to test it fails if a warning is present.

I am not terribly happy with this solution (feels really hacky rather than kinda hacky), so any alternative implementation ideas would be appreciated!

…or where it errors if libtock-rs has warnings.

This is a second attempt at PR tock#270. We can't specify RUSTFLAGS directly because doing so overrides all rustflags configurations in .cargo/config. Those rustflags configs are important because they configure relocation and set the linker script. Instead, I append the "deny warnings" config to .cargo/config in CI itself.
@jrvanwhy jrvanwhy added the significant Indicates a PR is significant as defined by the code review policy. label Feb 2, 2021
Copy link
Collaborator

@alistair23 alistair23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a little messy and will make it harder for people to see errors locally, but I agree that allowing warnings during development is useful. I usually end up with a local commit to do that.

@hudson-ayers
Copy link
Contributor

why will this make it harder to see errors locally?

@alistair23
Copy link
Collaborator

If someone is checking their changes locally warnings won't be errors.

@jrvanwhy
Copy link
Collaborator Author

jrvanwhy commented Feb 9, 2021

I think this PR is ready to merge.

@alistair23
Copy link
Collaborator

bors r+

@bors
Copy link
Contributor

bors bot commented Feb 9, 2021

Build succeeded:

@bors bors bot merged commit eff5851 into tock:master Feb 9, 2021
@jrvanwhy jrvanwhy deleted the allow-tmp-warnings branch May 5, 2021 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
significant Indicates a PR is significant as defined by the code review policy.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants